Skip to content

[DarkDragoon2002] iP#95

Open
DarkDragoon2002 wants to merge 42 commits into
nus-cs2113-AY2425S1:masterfrom
DarkDragoon2002:master
Open

[DarkDragoon2002] iP#95
DarkDragoon2002 wants to merge 42 commits into
nus-cs2113-AY2425S1:masterfrom
DarkDragoon2002:master

Conversation

@DarkDragoon2002

Copy link
Copy Markdown

No description provided.

1. Renamed Duke to Franz
2. Created the greeting and exit functions
3. Implemented the lines in between the messages
1. Renamed Franz to Juan
2. Implemented Echo feature
1. Added chatFeature Function to chat
2. Neatened Code
1. Creates Task Class
2. Shifted Functions from Main to Task class
3. Created functions to mark and unmark
1. Cleaned up Code
2. Reduced Nesting
3. Added Comments

@hzxnancy hzxnancy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code looks neat with the extraction of different functions and the use of constructors. The personalisation is very unique which makes this code fun to read, gran trabajo!

However, do take note of the naming of boolean variables.

Comment thread src/main/java/Juan.java Outdated
helloMessage();
lineMessage();

boolean continueChatting = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boolean variables/methods should be named to sound like booleans.
The same error is present in other parts of the code.

Comment thread src/main/java/Juan.java Outdated
Comment on lines +17 to +19
// Less efficient to create a new scanner everytime but code is much neater
// If return True means continue
// Else End

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks like a note to self and should not be here

Comment thread src/main/java/Juan.java Outdated
Comment on lines +9 to +12
boolean continueChatting = true;
while (continueChatting) {
continueChatting = chatFeature();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to extract these lines of code?

Comment thread src/main/java/Juan.java Outdated
Comment on lines +58 to +69
" ._-'-_ .\n" +
" . ' /_-_-_\\ ` .\n" +
" .' |-_-_-_-| `.\n" +
" ( `.-_-_-.' )\n" +
" !`. .'!\n" +
" ! ` . . ' !\n" +
" ! ! ! ! ! ! ! ! !\n" +
" / / \\ \\\n" +
" _-| \\___ ___/ /-_\n" +
" (_ )__\\_)\\(_/__( _)\n" +
" ))))\\X\\ ((((\n" +
" \\/ \\/ \n" +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this logo, it is very cute!

@Sukkaito Sukkaito left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, LGTM! 👍

Comment thread src/main/java/Juan.java Outdated
Comment on lines +57 to +69
String greeting =
" ._-'-_ .\n" +
" . ' /_-_-_\\ ` .\n" +
" .' |-_-_-_-| `.\n" +
" ( `.-_-_-.' )\n" +
" !`. .'!\n" +
" ! ` . . ' !\n" +
" ! ! ! ! ! ! ! ! !\n" +
" / / \\ \\\n" +
" _-| \\___ ___/ /-_\n" +
" (_ )__\\_)\\(_/__( _)\n" +
" ))))\\X\\ ((((\n" +
" \\/ \\/ \n" +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASCII art rules! 👍

Comment thread src/main/java/Juan.java Outdated
Comment on lines +25 to +46
if (line.equals("bye")) {
return false;
} else if (line.equals("list")) {
Task.printTasksList();
return true;
}

// Check for mark and unmark or just add task
String[] parts = line.split(" ");
if (parts[0].equals("mark")){
// Mark
int taskIndex = Integer.parseInt(parts[1]) - 1;
Task.mark(taskIndex);
} else if (parts[0].equals("unmark")){
// Unmark
int taskIndex = Integer.parseInt(parts[1]) - 1;
Task.unmark(taskIndex);
} else {
// else add task
Task newTask = new Task(line);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this part be extracted out?

Comment thread src/main/java/Task.java Outdated

// Keep track of tasks
private static Task[] tasks = new Task[100];
private static int taskNumber = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a better name can be made here, like taskCount?

Comment thread src/main/java/Task.java Outdated

// Object specific variables
public String taskString;
public boolean taskDone = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the boolean name here be more intuitive like in the Java coding standard?

Comment thread src/main/java/Juan.java Outdated
Comment on lines +52 to +55
public static void lineMessage() {
String line = "____________________________________________________________\n";
System.out.print(line);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the messages, should it be static variables instead and what the methods do is just printing it out?

Comment thread src/main/java/Juan.java Outdated
String line = scanner.nextLine();
lineMessage();

if (line.equals("bye")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May the command words also be variables, for later use or easier management?

Comment thread src/main/java/Juan.java Outdated
Comment on lines +56 to +69
public static void helloMessage() {
String greeting =
" ._-'-_ .\n" +
" . ' /_-_-_\\ ` .\n" +
" .' |-_-_-_-| `.\n" +
" ( `.-_-_-.' )\n" +
" !`. .'!\n" +
" ! ` . . ' !\n" +
" ! ! ! ! ! ! ! ! !\n" +
" / / \\ \\\n" +
" _-| \\___ ___/ /-_\n" +
" (_ )__\\_)\\(_/__( _)\n" +
" ))))\\X\\ ((((\n" +
" \\/ \\/ \n" +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the logo!

Comment thread src/main/java/Juan.java Outdated
byeMessage();
lineMessage();
}
public static boolean chatFeature(){

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's a method, should it be named with a verb?

@wenxin-c wenxin-c left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good job so far!! Just a few suggestions for you to consider.

Comment thread src/main/java/Juan.java Outdated
Comment on lines +17 to +22
// Continue chatting as long as user doesn't exit
boolean continueChatting = true;
while (continueChatting) {
// Chat feature to handle user input
continueChatting = chatFeature();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can consider extracting this part further to match same level of abstraction in this method.
E.g.

readData();
salary = basic * rise + 1000;
tax = (taxable ? salary * 0.07 : 0);
displayResult();

versus

readData();
processData();
displayResult();

Comment thread src/main/java/Juan.java Outdated
Task.printTasksList(); // Print task list from Task class
}
// Handle "mark" command to mark a task as completed
else if (line.startsWith("mark ")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid magic literals(e.g. numbers, strings), you can consider using named constants instead. It will be useful especially when the particular literals are used multiple times in the code.
E.g.

static final double PI = 3.14236;
static final int MAX_SIZE = 10;
...
return PI;
...
return MAX_SIZE - 1;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback just updated this!

Comment thread src/main/java/Juan.java Outdated
}

// Handles user input and executes corresponding actions
public static boolean chatFeature() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is very long, you can consider extracting the details for each case into separate methods. It will be better to keep each method short.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback just updated this! I extracted it into another class to make it easier to read

Comment thread src/main/java/Juan.java Outdated
Comment on lines +56 to +59
}
// Handle "unmark" command to unmark a task as completed
else if (line.startsWith("unmark ")) {
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to keep else if and closing curly bracket on the same line.
E.g.

if (condition) {
    statements;
} else if (condition) {
    statements;
} else {
    statements;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback just updated this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants